Skip to content

Fix NoSQL/SQL injection, prototype pollution and vulnerable dependencies - #283

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1789402286-fix-injection-proto-pollution
Open

devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1789402286-fix-injection-proto-pollution

Conversation

@devin-ai-integration

Copy link
Copy Markdown

Summary

Audit of the app for injection (SQL/NoSQL), prototype pollution and vulnerable dependencies, with one regression test file per fix (npm run test:unit, uses node:test, no DB required). Every exploit below was reproduced against main and confirmed closed against this branch with the app running.

1. NoSQL operator injection — POST /login (routes/index.js loginHandler)

Exploit: bodyParser.json() lets the client send objects instead of strings, so
{"username":"admin@snyk.io","password":{"$gt":""}} (or {"$gt":""} for both fields, no username needed) became User.find({ username, password: { $gt: "" } }) and matched the admin record → session loggedIn = 1.

Fix: require both fields to be typeof 'string' before touching the DB, and query with equality-only operators so nothing the client sends can ever be interpreted as an operator:

User.find({ username: { $eq: username }, password: { $eq: password } })

Test: tests/nosql-injection.test.js (asserts the DB is never queried for operator payloads and that the filter is exactly $eq).

2. Prototype pollution → privilege escalation — PUT /chat (routes/index.js chat.add/chat.delete)

Exploit: _.merge(message, req.body.message, ...) with lodash 4.17.4 walks __proto__, so
{"auth":{"name":"user","password":"pwd"},"message":{"__proto__":{"canDelete":true}}} set Object.prototype.canDelete = true. The regular user (who has no canDelete) then passed !user.canDelete and could DELETE /chat.

Fix: _.merge on untrusted input replaced with sanitizeMessage() (own keys only, __proto__/constructor/prototype skipped, string values only) + Object.assign; server-owned fields (id, timestamp, userName) are applied last so the client can't override them. chat.delete uses hasOwnProperty('canDelete') so an inherited value can never grant delete. lodash bumped to 4.18.1 as defense in depth.
Test: tests/prototype-pollution-chat.test.js.

3. Prototype pollution → SQL injection via TypeORM — POST /users / GET /users (routes/users.js)

Exploit (see exploits/prototype-pollution-typeorm.md): repo.save({ address: req.body.address }) with typeorm 0.2.24 deep-merged {"address":{"__proto__":{"where":{"id":"2","where":null}}}} into Object.prototype. The subsequent repo.find({ id: 1 }) then picked up the inherited where and returned arbitrary rows; the same primitive is the SQL injection in GHSA for typeorm ≤0.2.24 (CVE-2020-8158).

Fix: pickUserFields() whitelists name/address/role, requires own, string-valued properties, and returns 400 otherwise, so no nested object reaches the ORM. GET /users now passes an explicit find({ where: { id: 1 } }) (own property, so a polluted prototype can't shadow it). typeorm upgraded 0.2.24 → 0.3.31 (createConnection/getConnection → exported DataSource in typeorm-db.js).
Test: tests/typeorm-injection.test.js (stubs the DataSource; also asserts find still receives where: {id: 1} even with Object.prototype.where pre-polluted).

4. Insecure dependencies

package before after why
lodash 4.17.4 4.18.1 prototype pollution in merge/set, ReDoS, code injection
typeorm 0.2.24 0.3.31 prototype pollution → SQL injection in find/save/update
mongoose 4.2.4 8.24.4 remote memory exposure via Buffer cast ({"content":800}, see exploits/mongoose-exploits.sh), mquery code injection, prototype pollution via Schema.path, $nor sanitizeFilter bypass
express / body-parser 4.12.4 / 1.9.0 4.22.2 / 1.20.6 open redirect/XSS in res.redirect, vulnerable qs + path-to-regexp, urlencoded DoS
qs (transitive, via overrides) 6.x 6.16.0 prototype pollution via bracket notation, arrayLimit bypass DoS
dustjs-linkedin 2.5.0 3.0.1 prototype pollution in dust core
dustjs-helpers 1.5.0 removed {@if cond=…} is eval-based; ?device[]=Desktop'-require('child_process').exec(...)-' was RCE (exploits/dustjs-exploits.sh). about_new.dust now uses native {?isDesktop} with the boolean computed server-side, and device is coerced to a string.
mongodb, tap direct deps removed unused in app code; tap alone pulled ~400 packages incl. lodash 4.17.10, minimist, request…

Code adapted for mongoose 8 (callback API removed → promises; todo.remove(cb)findByIdAndDelete). Two related hardening changes in create: content must be a string (400 otherwise — closes the Buffer memory-exposure path independent of the mongoose version), and exec('identify ' + url)execFile('identify', [url]) with validator.isURL so a crafted image URL can no longer inject shell commands.
Tests: tests/vulnerable-dependencies.test.js (minimum-version assertions + live _.merge pollution check) and tests/todo-create.test.js.

Verification

  • npm run test:unit → 22/22 pass.
  • App boots against a local mongod (MySQL absent → logs the same connection error as before); /, /about_new, /login, /chat, /create, /users exercised with the payloads from exploits/ → all return 400/401/403 and Object.prototype stays clean.
  • npm audit --omit=dev: 76 → 36 vulnerable packages; typeorm/mongoose/lodash/express/body-parser/qs/dust are clean.

Out of scope / follow-ups (still flagged by npm audit, not injection/proto-pollution related)

adm-zip (zip-slip), st (path traversal), ms/humanize-ms/moment/validator/marked (ReDoS/XSS), ejs/ejs-locals/hbs (template RCE), express-fileupload, cfenv, npmconf, errorhandler, morgan, jquery. Note this is Snyk's vulnerable-by-design demo app, so some of these may be intentionally retained.

Devin-Org: engineering

Link to Devin session: https://app.devin.ai/sessions/f5d251a253b048c8b80ea73e0eec29d6
Open in Devin Desktop: https://app.devin.ai/desktop/session/f5d251a253b048c8b80ea73e0eec29d6?variant=devin
Requested by: @rushcromer


Note

Devin errored when opening this Pull Request as rushcromer.
As a fallback, Devin opened this PR as itself.

…ble dependencies

- loginHandler: reject non-string credentials, use $eq filters (NoSQL operator injection)
- /chat: replace lodash.merge on untrusted body with sanitized Object.assign, own-property canDelete check
- /users: whitelist string columns before repo.save, explicit where clause; typeorm 0.2.24 -> 0.3.31 (DataSource)
- /create: reject non-string content (mongoose Buffer memory exposure), execFile instead of exec for identify
- about_new: drop eval-based dustjs-helpers @if; dustjs-linkedin 2.5.0 -> 3.0.1
- lodash 4.17.4 -> 4.18.1, mongoose 4.2.4 -> 8.24.4, express 4.12.4 -> 4.22.2, body-parser 1.9.0 -> 1.20.6, qs override 6.16.0
- remove unused mongodb and tap dependencies
- add node:test regression tests (npm run test:unit)

Co-Authored-By: Rush Cromer II <rush.cromerii@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants